AVRO-1891: Fix specific nested logical types - #118
Conversation
AVRO-1884 changed makePath to a private method from a package-private static method. This broke the test that references the method in IPC. The fix is to make the instance method package-private and update the test to use an instance of SpecificCompiler.
This adds getConversionFor and getConversionByClass to GenericDatumReader so that SpecificDatumReader can override them to resolve conversions using the set of conversions used when a specific class was compiled, determined by the CONVERSIONS static variable or by adding the conversions for each field returned by getConversion(String).
This adds a CONVERSIONS static field with the converisons that the specific compiler used to produce a record. Together with the last commit, this will enable new specific classes to use nested logical types.
| if (logicalType == null) { | ||
| return null; | ||
| } | ||
| Conversion<?> conversion = conversions.get().peekLast() |
There was a problem hiding this comment.
peekLast() could return null. We need to check it to avoid NPE.
|
And also, we would need to change the datum writers to support the conversions, do we? |
|
Yes, this just shows how I think we could fix the problem. We still need to finish the write side and add tests. |
|
Hi, Thanks |
|
Hi @rdblue and @yibing-shi , any updates on this? is it the same as https://stackoverflow.com/questions/45581437/how-to-specify-converter-for-default-value-in-avro-union-logical-type-fields ? |
| protected static final org.apache.avro.Conversions.DecimalConversion DECIMAL_CONVERSION = new org.apache.avro.Conversions.DecimalConversion(); | ||
| #end | ||
|
|
||
| public static final Collection<Conversion<?>> CONVERSIONS = new ArrayList<Conversion<?>>(); |
There was a problem hiding this comment.
There are missing imports from the template:
import java.util.ArrayList;
import java.util.Collection;
import org.apache.avro.Conversion;
What is CONVERSIONS used for? Is this needed?
|
@rdblue I think think the template is not yet processed correctly. It seems, that for union schema, SpecificCompiler#hasLogicalTypeField returns false even when the union has logical types, due to this the conversion fields are not included in the compiled class. Should it instead check each type in the union? Something like this: public boolean hasLogicalTypeField(Schema schema) { |
|
@rdblue are you still working on this? |
|
This won't work for unions, because a union field may have multiple logical types by definition... |
|
What happened to this? This seems pretty major, because without it, unions with built-in logical types are broken. |
|
@wheelerlaw, feel free to pick this up. I'll help review the changes. |
|
Is there still any intention of finishing & merging this PR? I ran into this today when trying to serialize avro data to a Kafka stream. |
|
Hey @lewisdawson, there is a workaround for the current version that might help you. It involves implementing your own Avro serializer (or slightly modifying the one you're using, I did that with Confluent's). The key is to explicitly add the right conversions for logical types to SpecificData when creating the DatumWriter and DatumReader. If you still need help, create a post on StackOverflow and we'll take it over there. |
…on the write side of things as well as tests. See comments here - https://issues.apache.org/jira/browse/AVRO-1891?focusedCommentId=15465679&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-15465679
|
@scatrin can you follow up on that? it seems like the problem is still ongoing despite the effort from Avro community I wonder if it's possible to do in user space, without building the custom version of avro library |
|
@lazyval The workaround that I did applies to a setting with Kafka and Confluent's Schema Registry. The trick is to modify the serializer/deserializers (from Confluent) to explicitly register the needed conversions when creating the SpecificDatumWriter/Reader. (for instance here). SpecificDatumWriter has a getter for the SpecificData instance and SpecificData (via its superclass) has a method for registering logical type conversions. The cool thing is that the conversion class does not have to be part of Avro, you can build your own if you want to. Then just make sure that the modified classes are in the classpath when serializing/deserializing and that it is referenced properly in the Kafka settings. |
|
I merged #329, is this not needed now? |
Correcting cache annotation for classpath
13 participants
This changes the datum readers to use
getConversionForandgetConversionByClassinstance methods that delegate to the data models. This allows specific to use a set of conversions determined by the current record class to pick up the conversions that were present when it was compiled.